[client-v2, jdbc-v2] Handling of unknown parameters#2688
Closed
[client-v2, jdbc-v2] Handling of unknown parameters#2688
Conversation
…ltering out JDBC properties from client ones
…stom_ properties to server
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Other comments (2)
- jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java (65-65) There's a typo in the variable name 'driverPropertiesMapBuidler' which should be 'driverPropertiesMapBuilder'.
-
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcConfigurationTest.java (125-125)
The assertion message uses direct concatenation which could lead to confusing output. Consider using a formatted string instead:
assertEquals(configuration.clientProperties, expectedClientProps, String.format("clientProperties mismatch: %s vs %s", configuration.clientProperties, expectedClientProps));
💡 To request another review, post a new comment with "/windsurf-review".
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java
Outdated
Show resolved
Hide resolved
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
There was a problem hiding this comment.
Pull request overview
This PR introduces validation for unknown configuration parameters in both the client-v2 and jdbc-v2 libraries. The main change is to throw an exception when an unknown parameter is detected during client build, with an opt-out mechanism via the no_throw_on_unknown_config property. Additionally, the PR fixes how custom parameters are passed to the server (requiring custom_ prefix) and ensures JDBC driver-specific parameters are not incorrectly forwarded to the client.
Key Changes:
- Added validation to throw
ClientMisconfigurationExceptionon unknown config properties unless bypassed - Fixed parameter handling to distinguish between driver-only and client properties
- Corrected custom settings to use proper prefixing for server settings
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java | Added validation logic to throw on unknown config properties; added CUSTOM_SETTINGS_PREFIX property and NO_THROW_ON_UNKNOWN_CONFIG constant |
| client-v2/src/main/java/com/clickhouse/client/api/ServerException.java | Added UNKNOWN_SETTING error code constant |
| client-v2/src/main/java/com/clickhouse/client/api/Client.java | Removed extra blank lines |
| client-v2/src/test/java/com/clickhouse/client/ClientTests.java | Added test coverage for unknown client settings validation |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/DriverProperties.java | Added helper methods for server settings and HTTP headers |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java | Refactored property initialization to separate driver properties from client properties |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcConfigurationTest.java | Updated test data to use custom_ prefix and verify driver properties aren't passed to client |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/DriverTest.java | Added comprehensive test for unknown settings validation; changed assertions to use static imports |
| clickhouse-r2dbc/src/test/java/com/clickhouse/r2dbc/spi/test/R2DBCTestKitImplTest.java | Fixed property name from serverTimezone to server_time_zone |
Comments suppressed due to low confidence (1)
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java:1
- Missing space after the '?' in the ternary operator. For consistency, add a space:
isCloud() ? \"SQL_\" :instead ofisCloud()? \"SQL_\" :
package com.clickhouse.client.api;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java
Outdated
Show resolved
Hide resolved
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
.github/workflows/nightly.yml:1
- Commented-out code should be removed. This bash script appears to be dead code that serves no purpose in the workflow.
name: Nightly
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcConfigurationTest.java
Outdated
Show resolved
Hide resolved
|
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java
Show resolved
Hide resolved
Contributor
|
@chernser, what was exactly changed in |
Contributor
Author
|
@mzitnik spotless reformatted. Will revert. |
Contributor
|
let's have a separate PR for spotless formatting since it is confusing. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
no_throw_on_unknown_configto properties. Parsing happens on.build()custom_sslas it is only for driver)Closes #2658
Checklist
Delete items not relevant to your PR:
Note
Introduces stricter and clearer client/JDBC configuration handling.
ClientConfigProperties.parseConfigMapnow throws on unknown properties (opt-out viano_throw_on_unknown_config); addsCUSTOM_SETTINGS_PREFIX(defaultcustom_) and maps those keys to server settings.JdbcConfigurationno longer forwards driver-only props (e.g.,ssl) to the client; sets driver defaults and keeps driver/client props distinct.ServerException.UNKNOWN_SETTINGconstant; tests cover unknown client/server settings and invalid prefix.server_time_zone; update client builder to react to option keys.-Dspotless.skip=true; nightly/release workflows updated accordingly.Written by Cursor Bugbot for commit 5987bbf. This will update automatically on new commits. Configure here.